Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New WebUI: Update angular and angular-route and add workaround to be compatible #1813

Merged
merged 4 commits into from
Feb 10, 2023
Merged

New WebUI: Update angular and angular-route and add workaround to be compatible #1813

merged 4 commits into from
Feb 10, 2023

Conversation

kripton
Copy link
Member

@kripton kripton commented Feb 6, 2023

This PR updates angular and angular-route to the latest upstream releases (v1.8.3). However, without changes to our new Web UI, this causes it to break as described in #1782. To fix this, a workaround has been added to restore angular's previous behaviour. See https://docs.angularjs.org/guide/migration#commit-aa077e8

As a bonus, the list of dependencies in javascript/new-src/package.json is not sorted alphabetically as is done automatically when adding or updating packages listed therein.

This PR also updates the "built" app.min.js and related files.

@kripton kripton added this to the 0.10.9 milestone Feb 6, 2023
@kripton
Copy link
Member Author

kripton commented Feb 6, 2023

Oh, forgot: This PR also updates jQuery to the latest upstream release.

I did test the new Web UI after the changes:

  • Navigating
  • Changing values via faders
  • Changing values via keypad
  • Adding a new universe
  • Changing universe patching
  • Disabling and Re-enabling a plugin
  • Requesting plugin reload

All functions seem to work as before

@FloEdelmann
Copy link
Member

Oops, sorry

FloEdelmann
FloEdelmann approved these changes Feb 6, 2023
View reviewed changes
Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look fine. I haven't checked out the code locally though.

@FloEdelmann
Copy link
Member

Looks like the Travis CI job doesn't even start. Should we merge the PR manually without that check?

daveol
daveol approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@daveol daveol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes seem fine, Haven't tested it in the real world though.

@daveol
Copy link
Contributor

daveol commented Feb 7, 2023

Travis CI is giving the following error on the site (when i logged in at least):

    We are unable to start your build at this time. 
    You exceeded the number of users allowed for your plan.
    Please review your plan details and follow the steps to resolution.

peternewman
peternewman approved these changes Feb 8, 2023
View reviewed changes
Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with others LGTM but I've not tried it.

Just the one possible comment improvement.

javascript/new-src/src/app.js
Comment on lines +23 to +25
// AngularJS 1.6 now uses an '!' as the default hashPrefix, breaking
// our routes. Following lines revert the default to the previous empty
// string.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could also add a note saying given this webpage is only used internally, there is no benefit in the web crawler changes.

We could possibly add a TODO to update our URLs to remove this change, but maybe that's not necessary if we're rewriting to a new framework in future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this might be useful but since AngularJS (= 1.x) is a dead end, we won't bother for now

@peternewman
Copy link
Member

Looks like the Travis CI job doesn't even start.

Travis CI is giving the following error on the site (when i logged in at least):

    We are unable to start your build at this time. 
    You exceeded the number of users allowed for your plan.
    Please review your plan details and follow the steps to resolution.

See e.g. https://travis-ci.community/t/false-statement-builds-have-been-temporarily-disabled-for-private-and-public-repositories-due-to-a-negative-credit-balance/13557/4

You could try going here @kripton and logging into Travis:
https://app.travis-ci.com/organizations/OpenLightingProject/plan/usage

Maybe this is just a guerrilla attempt to force more people to GitHub Actions?

Maybe we should work on this for Linux too:
#1809

Should we merge the PR manually without that check?

Quite possibly, the tests won't really check it aside from the JS linting stuff which I guess you've run?

Or do a dummy commit and see if it does anything the second time around.

@DaAwesomeP
Copy link
Member

@peternewman I'm happy to work on a Linux build for GitHub Actions to compliment #1809 and #1812 to fully replace Travis.

@DaAwesomeP DaAwesomeP mentioned this pull request Feb 9, 2023
Open
51 tasks
@kripton
Copy link
Member Author

kripton commented Feb 10, 2023

You could try going here @kripton and logging into Travis: https://app.travis-ci.com/organizations/OpenLightingProject/plan/usage

I did. It gave me some "plan exceeded" limit, mainly regarding the "number of users on the plan". However, I am unsure if it meant my personal GitHub account or the OpenLightingProject organization.
I would also say let's try to get rid of Travis and do everything with GH Actions. @DaAwesomeP: Thanks for picking it up!

With all the positive review feedbacks here and since Travis won't catch errors in JavaScript anyways, I'll just merge this one.

@kripton kripton merged commit 591b9d5 into OpenLightingProject:0.10 Feb 10, 2023
@kripton kripton deleted the 0.10-UpdateNewWebUI branch February 10, 2023 20:25
@kripton kripton mentioned this pull request Feb 10, 2023
Closed
@peternewman peternewman mentioned this pull request Feb 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peternewman peternewman peternewman approved these changes

@daveol daveol daveol approved these changes

@FloEdelmann FloEdelmann FloEdelmann approved these changes

@nomis52 nomis52 Awaiting requested review from nomis52

Assignees
No one assigned
Labels
bug Component-UI Language-JS
Projects
None yet
Milestone
0.10.9
Development

Successfully merging this pull request may close these issues.
5 participants
@kripton @FloEdelmann @daveol @peternewman @DaAwesomeP